Add rate limit to Port client #262

erikzaadi · 2025-06-23T18:45:22Z

Description

What - Add rate limit capabilities to the base port http client
Why - Prevent failing on multiple 429
How - Add resty middle ware that detects rate limits and acts accordingly

Type of change

Please leave one option from the following and delete the rest:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Non-breaking change (fix of existing functionality that will not change current behavior)
Documentation (added/updated documentation)

github-actions · 2025-06-23T19:12:21Z

	Tests	Passed ✅	Skipped	Failed	Time ⏱
JUnit Test Report	134 ran	134 ✅	0 ⚠️	0 ❌	14m 31s 598ms

Test	Retries	Time ⏱
JUnit Test Report
com/port-labs/terraform-provider-port-labs/v2/port/entity.TestAccPortEntityWithManyRelation	1	10s 230ms
com/port-labs/terraform-provider-port-labs/v2/port/team.TestAccPortTeamEmptyDescription	1	9s 180ms
com/port-labs/terraform-provider-port-labs/v2/port/team.TestAccPortTeamUpdate	1	13s 290ms
com/port-labs/terraform-provider-port-labs/v2/port/team.TestAccPortTeamImport	1	6s 870ms
com/port-labs/terraform-provider-port-labs/v2/port/team.TestAccPortTeam	1	4s 160ms

View JUnit Test Report

github-actions · 2025-06-23T19:12:23Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/15832472715/artifacts/3386161105

Code Coverage Total Percentage: `41.5%`

github-actions · 2025-06-23T19:12:25Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 41.5

github-actions · 2025-06-23T19:29:10Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/15832678263/artifacts/3386278375

Code Coverage Total Percentage: `63.1%`

github-actions · 2025-06-23T19:29:13Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 63.1

Eyal-Shalev

There are 2 things I don't like about this PR:

You are implementing a rate limit yourself which seems an unnecessary complication. I'm sure there are great libraries for something this important.
The rate limit is set globally. Shouldn't it only affect retries on the same request?

internal/cli/models.go

github-actions · 2025-06-24T09:36:22Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/15846001956/artifacts/3390640592

Code Coverage Total Percentage: `66.1%`

github-actions · 2025-06-24T09:36:24Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 66.1

github-actions · 2025-06-24T09:38:32Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/15846038502/artifacts/3390655240

Code Coverage Total Percentage: `78.1%`

github-actions · 2025-06-30T07:28:43Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/15965961435/artifacts/3429784483

Code Coverage Total Percentage: `50.6%`

github-actions · 2025-06-30T07:28:45Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 50.6

github-actions · 2025-07-06T06:25:31Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16095891109/artifacts/3471447766

Code Coverage Total Percentage: `63.4%`

github-actions · 2025-07-06T06:25:33Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 63.4

github-actions · 2025-07-06T08:21:05Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16096719592/artifacts/3471644206

Code Coverage Total Percentage: `66.2%`

github-actions · 2025-07-06T08:21:06Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 66.2

github-actions · 2025-07-06T09:48:24Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16096719592/artifacts/3471797920

Code Coverage Total Percentage: `66.2%`

github-actions · 2025-07-06T09:48:26Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 66.2

github-actions · 2025-07-06T10:30:31Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16097790940/artifacts/3471874623

Code Coverage Total Percentage: `63.2%`

github-actions · 2025-07-06T10:30:33Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 63.2

github-actions · 2025-07-07T07:18:41Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16109839014/artifacts/3474717624

Code Coverage Total Percentage: `66.6%`

github-actions · 2025-07-07T07:18:43Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 66.6

Eyal-Shalev · 2025-07-07T08:11:02Z

internal/cli/client.go

+// WithRateLimitThreshold sets the threshold for when to start throttling
+// threshold should be between 0.0 and 1.0 (e.g., 0.1 means start throttling when 10% of requests remain)
+func WithRateLimitThreshold(threshold float64) Option {
+	return func(pc *PortClient) {
+		if threshold >= 0.0 && threshold <= 1.0 {
+			pc.rateLimitManager.SetThreshold(threshold)
+		}
+	}
+}


This is too obscure. I think that you should return an error or panic if it is out of bounds.

Another option is to clamp the threshold between 0 and 1.

internal/ratelimit/ratelimit.go

Eyal-Shalev · 2025-07-07T08:27:50Z

internal/ratelimit/ratelimit.go

+		m.mu.Lock()
+		m.activeRequests--
+		if m.activeRequests < 0 {
+			m.activeRequests = 0 // Prevent negative count


I would log stuff like this as it shows we have a bug in our code

internal/ratelimit/ratelimit.go

github-actions · 2025-07-13T13:00:07Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16249103961/artifacts/3521483633

Code Coverage Total Percentage: `63.9%`

github-actions · 2025-07-13T17:32:11Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 66.8

internal/cli/client_ratelimit_test.go

internal/ratelimit/ratelimit.go

Co-authored-by: Eyal Halpern Shalev <[email protected]>

github-actions · 2025-07-14T13:16:50Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16266967554/artifacts/3526735039

Code Coverage Total Percentage: `76.2%`

github-actions · 2025-07-14T13:26:09Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16266979758/artifacts/3526813956

Code Coverage Total Percentage: `72.5%`

github-actions · 2025-07-14T13:36:24Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16267878717/artifacts/3526902115

Code Coverage Total Percentage: `76.4%`

github-actions · 2025-07-14T14:16:14Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16268452431/artifacts/3527252617

Code Coverage Total Percentage: `67.1%`

github-actions · 2025-07-14T14:16:16Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 67.1

github-actions · 2025-07-14T14:22:04Z

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16268263599/artifacts/3527308241

Code Coverage Total Percentage: `63.5%`

github-actions · 2025-07-14T14:22:06Z

🚨 The new code coverage percentage is lower than the current one. Current coverage: 71.4
While the new one is: 63.5

Eyal-Shalev

Eyal-Shalev · 2025-07-15T12:32:11Z

internal/ratelimit/ratelimit.go

+		// Use recover to catch any potential panics from semaphore operations
+		if r := recover(); r != nil {
+			m.logger.Debug("Recovered from panic in ResponseMiddleware defer", "panic", r)
+		}


For this to work you need to wrap it inside a new defer func() {...}

Suggested change

// Use recover to catch any potential panics from semaphore operations

if r := recover(); r != nil {

m.logger.Debug("Recovered from panic in ResponseMiddleware defer", "panic", r)

}

defer func() {

// Use recover to catch any potential panics from semaphore operations

if r := recover(); r != nil {

m.logger.Debug("Recovered from panic in ResponseMiddleware defer", "panic", r)

}

}

Eyal-Shalev · 2025-07-15T12:37:08Z

internal/ratelimit/ratelimit.go

+	// Try to acquire semaphore immediately (non-blocking) as load indicator
+	ctx, cancel := context.WithTimeout(context.Background(), 0)
+	defer cancel()
+
+	semaphoreAcquired := false
+	if err := m.requestSemaphore.Acquire(ctx, 1); err != nil {
+		m.logger.Warn("High concurrent load detected - proceeding anyway")
+	} else {
+		m.logger.Debug("Normal load - acquired semaphore slot")
+		semaphoreAcquired = true
+	}


I thought you wanted to have a timeout on the Acquire, if you don't you can just use TryAcquire instead.

Suggested change

// Try to acquire semaphore immediately (non-blocking) as load indicator

ctx, cancel := context.WithTimeout(context.Background(), 0)

defer cancel()

semaphoreAcquired := false

if err := m.requestSemaphore.Acquire(ctx, 1); err != nil {

m.logger.Warn("High concurrent load detected - proceeding anyway")

} else {

m.logger.Debug("Normal load - acquired semaphore slot")

semaphoreAcquired = true

}

semaphoreAcquired := m.requestSemaphore.TryAcquire(1)

if semaphoreAcquired {

m.logger.Debug("Normal load - acquired semaphore slot")

} else {

m.logger.Warn("High concurrent load detected - proceeding anyway")

}

Eyal-Shalev · 2025-07-15T12:46:04Z

internal/ratelimit/ratelimit.go

+		activeRequests := m.activeRequests.Add(-1)
+		if activeRequests < 0 {
+			// Reset to 0 if somehow went negative
+			m.activeRequests.Store(0)
+			activeRequests = 0
+		}


This isn't atomic as well.

I see you choose to use a mutex, so just use it everywhere you do a check-and-update for m.activeRequests and change m.activeRequests to be a normal int.

Eyal-Shalev · 2025-07-15T13:29:47Z

internal/ratelimit/ratelimit.go

+// contextKey is a custom type for context keys to avoid collisions
+type contextKey string


I don't think this comment is needed.

Suggested change

// contextKey is a custom type for context keys to avoid collisions

type contextKey string

type contextKey string

Eyal-Shalev · 2025-07-15T13:36:59Z

internal/cli/client_ratelimit_test.go

+	assert.Nil(t, rateLimitInfo)
+}
+
+// Test-specific option functions


Remove comment. This is a test file so of course it is test specific.

Suggested change

// Test-specific option functions

Eyal-Shalev · 2025-07-15T14:32:24Z

internal/cli/client_ratelimit_test.go

+func TestClientRateLimitDisabled(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("x-ratelimit-limit", "1000")
+		w.Header().Set("x-ratelimit-remaining", "1")
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte(`{"ok": true}`))
+	}))
+	defer server.Close()
+
+	client, err := New(server.URL, WithRateLimitDisabled())
+	require.NoError(t, err)
+
+	resp, err := client.Client.R().Get("/test")
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusOK, resp.StatusCode())
+
+	rateLimitInfo := client.RateLimitManager.GetInfo()
+	assert.Nil(t, rateLimitInfo)
+}


This is already tested in ratelimit package. No need to double test things.

Eyal-Shalev · 2025-07-15T14:33:18Z

internal/cli/client_ratelimit_test.go

+func TestClientRateLimitThrottling(t *testing.T) {
+	skipIfDisabled(t)
+
+	requestCount := 0
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		requestCount++
+		w.Header().Set("x-ratelimit-limit", "100")
+		w.Header().Set("x-ratelimit-remaining", "5")
+		w.Header().Set("x-ratelimit-reset", "2")
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte(`{"ok": true}`))
+	}))
+	defer server.Close()
+
+	client, err := New(server.URL, WithRateLimitThreshold(0.1))
+	require.NoError(t, err)
+
+	start := time.Now()
+	resp, err := client.Client.R().Get("/test1")
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusOK, resp.StatusCode())
+
+	resp, err = client.Client.R().Get("/test2")
+	elapsed := time.Since(start)
+
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusOK, resp.StatusCode())
+	assert.Equal(t, 2, requestCount)
+
+	assert.Greater(t, elapsed, 10*time.Millisecond, "Request should have been throttled")
+}


This is already tested in ratelimit package. No need to double test things.

Eyal-Shalev · 2025-07-15T14:37:10Z

internal/ratelimit/ratelimit_test.go

+	t.Run("with nil options uses defaults", func(t *testing.T) {
+		manager := NewManager(nil)
+		assert.NotNil(t, manager)
+		assert.NotNil(t, manager.logger)
+		assert.Equal(t, 0.02, manager.threshold)
+		assert.Equal(t, 50*time.Millisecond, manager.minRequestInterval)
+		assert.Equal(t, 30*time.Second, manager.cleanupInterval)
+	})


I don't think this is needed. But if you do test the defaults, please use const-s for the hard coded values. Or at least call the function that creates a default options struct.

Eyal-Shalev · 2025-07-15T14:38:35Z

internal/ratelimit/ratelimit_test.go

+func TestDefaultManagerOptions(t *testing.T) {
+	defaults := DefaultManagerOptions()
+
+	assert.NotNil(t, defaults.Logger)
+	assert.NotNil(t, defaults.Threshold)
+	assert.Equal(t, 0.02, *defaults.Threshold)
+	assert.NotNil(t, defaults.SemaphoreWeight)
+	assert.Equal(t, int64(50), *defaults.SemaphoreWeight)
+	assert.NotNil(t, defaults.MinRequestInterval)
+	assert.Equal(t, 50*time.Millisecond, *defaults.MinRequestInterval)
+	assert.NotNil(t, defaults.CleanupInterval)
+	assert.Equal(t, 30*time.Second, *defaults.CleanupInterval)
+}


Didn't we already test the defaults in the above test?

Eyal-Shalev · 2025-07-15T14:40:21Z

internal/ratelimit/ratelimit_test.go

+	semaphoreWeight := int64(1)
+	manager := NewManager(&ManagerOptions{
+		SemaphoreWeight: &semaphoreWeight,
+		Logger:          logger,
+	})


I think you can do stuff like this.

Suggested change

semaphoreWeight := int64(1)

manager := NewManager(&ManagerOptions{

SemaphoreWeight: &semaphoreWeight,

Logger: logger,

})

manager := NewManager(&ManagerOptions{

SemaphoreWeight: toPtr(1),

Logger: logger,

})

erikzaadi force-pushed the PORT-15207-rate-limit branch from 99b4284 to 01d6aa3 Compare June 23, 2025 18:56

Eyal-Shalev reviewed Jun 24, 2025

View reviewed changes

internal/cli/models.go Outdated Show resolved Hide resolved

erikzaadi force-pushed the PORT-15207-rate-limit branch from 2bc681c to 889d9e2 Compare June 24, 2025 08:59

erikzaadi requested a review from Eyal-Shalev June 30, 2025 14:57

erikzaadi force-pushed the PORT-15207-rate-limit branch from 875daef to 903a17d Compare July 6, 2025 05:52

erikzaadi force-pushed the PORT-15207-rate-limit branch from 2ec5266 to bd1e329 Compare July 7, 2025 06:44

Eyal-Shalev requested changes Jul 7, 2025

View reviewed changes

erikzaadi force-pushed the PORT-15207-rate-limit branch from bd1e329 to 51cf48e Compare July 13, 2025 12:25

Eyal-Shalev reviewed Jul 14, 2025

View reviewed changes

internal/cli/client_ratelimit_test.go Outdated Show resolved Hide resolved

Eyal-Shalev requested changes Jul 14, 2025

View reviewed changes

erikzaadi and others added 15 commits July 14, 2025 15:35

Add rate limit to Port client

9de4005

[PR-FIX] Remove json info on RateInfo model

6a5dd83

[PR-FIX] Mutex

84a73f5

[PR-FIX] De-cursorify

16e434d

[DROP] Test for change

5767219

[SQUASH] Fix tests due to StupidErikException

5f08bf6

[PR-FIX] Proper comments when needed

0e4d1c3

[PR-Fixes] Golang semaphore and comments

1ef35ee

[PR-FIX] Saner logger

6c46baa

[PR-FIX] Proper jitter

4900ee8

[PR-FIX] add cleanup to fix potential rift

574b665

[PR-FIX] Saner locks

fdaa146

[PR-FIX] Add Test Helper

03e050f

Co-authored-by: Eyal Halpern Shalev <[email protected]>

[PR-FIX] Proper dochead

f402bd5

Co-authored-by: Eyal Halpern Shalev <[email protected]>

[PR-FIX][SQUASH] ZE THINGZ

3d0ce6e

erikzaadi force-pushed the PORT-15207-rate-limit branch from bb27428 to 3d0ce6e Compare July 14, 2025 13:14

erikzaadi added 2 commits July 14, 2025 16:31

[PR-FIX] Cleanup ze thingz

21ee431

[SQUASH] Missed a read lock

a7b14a4

[SQUASH] proper test separations

6f18612

Eyal-Shalev requested changes Jul 15, 2025

View reviewed changes

		// contextKey is a custom type for context keys to avoid collisions
		type contextKey string

Add rate limit to Port client #262

Are you sure you want to change the base?

Add rate limit to Port client #262

Uh oh!

Conversation

erikzaadi commented Jun 23, 2025

Description

Type of change

Uh oh!

github-actions bot commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 23, 2025

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/15832472715/artifacts/3386161105

Code Coverage Total Percentage: 41.5%

Uh oh!

github-actions bot commented Jun 23, 2025

Uh oh!

github-actions bot commented Jun 23, 2025

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/15832678263/artifacts/3386278375

Code Coverage Total Percentage: 63.1%

Uh oh!

github-actions bot commented Jun 23, 2025

Uh oh!

Eyal-Shalev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jun 24, 2025

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/15846001956/artifacts/3390640592

Code Coverage Total Percentage: 66.1%

Uh oh!

github-actions bot commented Jun 24, 2025

Uh oh!

github-actions bot commented Jun 24, 2025

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/15846038502/artifacts/3390655240

Code Coverage Total Percentage: 78.1%

Uh oh!

github-actions bot commented Jun 30, 2025

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/15965961435/artifacts/3429784483

Code Coverage Total Percentage: 50.6%

Uh oh!

github-actions bot commented Jun 30, 2025

Uh oh!

github-actions bot commented Jul 6, 2025

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16095891109/artifacts/3471447766

Code Coverage Total Percentage: 63.4%

Uh oh!

github-actions bot commented Jul 6, 2025

Uh oh!

github-actions bot commented Jul 6, 2025

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16096719592/artifacts/3471644206

Code Coverage Total Percentage: 66.2%

Uh oh!

github-actions bot commented Jul 6, 2025

Uh oh!

github-actions bot commented Jul 6, 2025

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16096719592/artifacts/3471797920

Code Coverage Total Percentage: 66.2%

Uh oh!

github-actions bot commented Jul 6, 2025

Uh oh!

github-actions bot commented Jul 6, 2025

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16097790940/artifacts/3471874623

Code Coverage Total Percentage: 63.2%

Uh oh!

github-actions bot commented Jul 6, 2025

Uh oh!

github-actions bot commented Jul 7, 2025

Code Coverage Artifact 📈: https://github.com/port-labs/terraform-provider-port-labs/actions/runs/16109839014/artifacts/3474717624

Code Coverage Total Percentage: 66.6%

Uh oh!

github-actions bot commented Jul 7, 2025

Uh oh!

Eyal-Shalev Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Eyal-Shalev Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 23, 2025 •

edited

Loading

Code Coverage Total Percentage: `41.5%`

Code Coverage Total Percentage: `63.1%`

Code Coverage Total Percentage: `66.1%`

Code Coverage Total Percentage: `78.1%`

Code Coverage Total Percentage: `50.6%`

Code Coverage Total Percentage: `63.4%`

Code Coverage Total Percentage: `66.2%`

Code Coverage Total Percentage: `66.2%`

Code Coverage Total Percentage: `63.2%`

Code Coverage Total Percentage: `66.6%`

Code Coverage Total Percentage: `63.9%`

Code Coverage Total Percentage: `76.2%`

Code Coverage Total Percentage: `72.5%`

Code Coverage Total Percentage: `76.4%`

Code Coverage Total Percentage: `67.1%`

Code Coverage Total Percentage: `63.5%`